Skip to content

ext/standard: applied fixers to improve test robustness - #23136

Open
NickSdot wants to merge 3 commits into
php:masterfrom
NickSdot:test/hardening/ext-standard
Open

ext/standard: applied fixers to improve test robustness#23136
NickSdot wants to merge 3 commits into
php:masterfrom
NickSdot:test/hardening/ext-standard

Conversation

@NickSdot

@NickSdot NickSdot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Asserts error class; extracted from: #22799

GitHub is lagging this, easier to review with: https://diffshub.com/php/php-src/pull/23136

@NickSdot
NickSdot marked this pull request as ready for review August 8, 2026 08:19
@NickSdot
NickSdot requested a review from bukka as a code owner August 8, 2026 08:19
@NickSdot

Copy link
Copy Markdown
Contributor Author

@bukka following your comment yesterday I reviewed this one once again, and wanted to double check.

  1. Out of 4,210 total lines, 13 (in 4 files) are affected by the same style-only thematic. Are you here okay with unifying those 13 lines with the rest or do you want me to make a manual follow up?
  2. And while we are on it... Currently line endings are untouched; some said output always should be \n, others don't wanted them to be touched. Would you prefer to convert to \n or nah?

I would make a manual follow up to get these addressed.

Files affected by the style-only thematic:
ext/standard/tests/array/gh22678.phpt
ext/standard/tests/filters/gh20370.phpt
ext/standard/tests/strings/gh20906_1.phpt
ext/standard/tests/strings/setlocale_null_byte.phpt

@NickSdot
NickSdot force-pushed the test/hardening/ext-standard branch 3 times, most recently from 6176c2d to ebced5a Compare August 19, 2026 14:08
@NickSdot
NickSdot force-pushed the test/hardening/ext-standard branch from ebced5a to f9206bf Compare August 21, 2026 18:25

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are loads of ZPP tests that should be removed, would make the diff smaller and reduce the test suite aswell.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a ZPP test and should be removed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto ZPP test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diito

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@NickSdot

Copy link
Copy Markdown
Contributor Author

There are loads of ZPP tests that should be removed, would make the diff smaller and reduce the test suite aswell.

@Girgias that's done in #23413 -- when merged I will rebase and resolve here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants